-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial Plugin version #4
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Anton Yarmolenko <[email protected]>
* chore: added github workflows * fix: scheme name * chore: test linter error * fix: linter error --------- Co-authored-by: Anton Yarmolenko <[email protected]>
* chore: added tests * chore: fixed warnings * chore: fixed swiftlint warnings in tests * chore: added unit_test workflow --------- Co-authored-by: Anton Yarmolenko <[email protected]>
let package = Package( | ||
name: "EDXMobileAnalytics", | ||
platforms: [ | ||
.iOS(.v16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we ditching iOS 15?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes openEdx was moved to iOS 16 in October here - before the plugin architecture was implemented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we are still supporting iOS 15 in our app and as far as I understand, this package will be included in our app which will force the app to have deployment target of iOS 16 at least. I think it should be discussed in a Slack thread with wider audience so that everyone from business, product and dev side is aligned on the decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this package will be included in the edX app, but after upstream develop (which is iOS 16 and swift 6) will only be merged into the 2u/develop marketplace. And yes, we could discuss this at the next tech meeting. Thanks.
guard shouldListenNotification(userinfo: userInfo) else { return } | ||
|
||
segmentAnalyticService?.receivedRemoteNotification(userInfo: userInfo) | ||
deepLinkManager.processLinkFrom(userInfo: userInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is push notification handling also in the scope of this repository/package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as #4 (comment)
Sources/EDXMobileAnalytics/Segment/SegmentAnalyticsServiceMock.swift
Outdated
Show resolved
Hide resolved
Co-Authored-By: Anton Yarmolenko <[email protected]>
@mta452 your feedback was addressed and/or commented. |
@rnr I have added minor suggestions on the comments. Other than that it looks good to me. I'll approve it once we reach an understanding. |
Added Initial code, github workflows and Tests